Improve clicking in diff view to enter staging/patch building#3985
Improve clicking in diff view to enter staging/patch building#3985stefanhaller wants to merge 24 commits into
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
5e84f7e to
a20477d
Compare
a20477d to
dc9c309
Compare
dc9c309 to
ff1e6a8
Compare
ff1e6a8 to
f6912c8
Compare
f6912c8 to
6b1431d
Compare
|
One things that's a little strange is that when looking at the diff of a commit, it takes one click (or enter keypress) to go to patch building, but then it takes two escape presses to get out again. It's weird and takes some getting used to, but I think it's a good price to pay because the feature is so useful. If it bothers us too much we can consider remembering where we came from, and have esc take you all the way out to commits in that case (which may also be confusing though). |
I added a commit that does this, I think I prefer it this way (needs more testing though). |
ebd7852 to
06db652
Compare
b45c992 to
bc1cf22
Compare
06db652 to
5355731
Compare
bc1cf22 to
15b6116
Compare
5355731 to
e7e49dd
Compare
15b6116 to
678280f
Compare
e7e49dd to
c5fc123
Compare
678280f to
03f1307
Compare
c5fc123 to
b51fdb7
Compare
03f1307 to
afa3b7f
Compare
b51fdb7 to
0c20f49
Compare
afa3b7f to
f104a15
Compare
0c20f49 to
4e21a09
Compare
f104a15 to
983f6f8
Compare
983f6f8 to
2b4ccbf
Compare
2b4ccbf to
afc8256
Compare
afc8256 to
cc822bc
Compare
f5a8dd6 to
30e625a
Compare
|
@stefanhaller how do I configure the I've got this in my config but pressing G on a line does nothing: |
|
@jesseduffield You also need |
Re-rendering a diff into a main view is asynchronous and lazy: the read loop fills the view a screenful at a time and refreshes as it goes. When debugging scroll-restore and flicker behaviour, the individual frames go by too fast to see. Setting LAZYGIT_SLOW_RENDER=<milliseconds> sleeps that long after each line is written, stretching the load out so the frames become visible. It has no effect when unset, so it's safe to leave in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…building This was already possible, but only when a file was selected, and it woudln't always land on the right line when a pager was used. Now it's also possible to do this for directories, and it jumps to the right line. At the moment this is a hack that relies on delta's hyperlinks, so it only works on lines that have hyperlinks (added and context). The implementation is very hacky for other reasons too (e.g. the addition of the weirdly named ClickedViewRealLineIdx to OnFocusOpts).
… clicked line This involves first switching to the commit files view, and then entering the clicked file from there.
…ll the way back out I *think* I like it better this way, but it needs more testing.
…ction Instead of a user config that always shows a selection on focus, the focused main view starts without a selection (matching master). Pressing <space> shows a selection in the middle of the view (no scrolling); pressing <esc> hides it again before falling through to exiting the view.
HyperLinkInLine read v.lines/v.viewLines without holding writeMutex, so it could race a concurrent re-render rebuilding the buffer. It also indexed v.lines by viewLines[y].linesY after only checking y against len(viewLines); since refreshViewLinesIfNeeded overwrites viewLines in place without truncating, the tail can hold stale entries pointing past a shrunk v.lines, giving an out-of-range panic while a shorter diff is still loading. Take writeMutex (as the sibling view methods do) and bounds-check linesY against len(v.lines), returning "no link" rather than panicking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The layout scrolls a view up if its origin is past the bottom of its content, to avoid showing blank space (e.g. after a resize). But it measures content height by the lines loaded so far, and command/pty tasks load asynchronously. So when a view is re-rendered while scrolled down, the layout would yank it to the top because only a fraction of the content has been read yet, then leave it there once loading finished. Track whether a command task is actively reading (set synchronously when the task is created, so a layout pass in between sees it; cleared at EOF, but not when stopped, since that means a newer task is taking over) and skip the scroll-up clamp for such views. onEndOfInput already re-clamps once loading completes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A task's read loop processes one LinesToRead request at a time. The initial request has a large line count and no Then callback; if the content is shorter than that, the loop hits EOF on the initial request and breaks out, abandoning any further requests still sitting in the readLines channel. So a ReadToEnd call that races a still-loading-but-shorter-than-its-initial-read view has its Then silently dropped: it isn't fired immediately (the channel was non-nil at call time) and it's never dequeued. On EOF, drain the queued requests and fire their Then callbacks before breaking out, since reaching EOF trivially satisfies any "read more" request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Records session 2's findings: the escape flicker was caused by the layout's scroll-up-to-fill clamp running against partially-loaded async content (not the "only a screenful loaded" mechanism session 1 guessed); the full origin-reset chain (onNewKey / CopyContent / layout clamp) and how each is handled; the three standalone bug fixes that landed; and the precise remaining task (apply the saved origin at the pty task's first repaint, i.e. a cmd-task RenderWithScroll). Also captures the reusable debug tooling that was stripped from the tree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Several methods assigned v.ox and v.oy directly: SetOrigin, CopyContent,
the wrap/autoscroll branches in draw, FocusPoint, and
Scroll{Up,Down,Left,Right}. Funnelling them all through SetOriginX and
SetOriginY gives a single place to observe (or set a breakpoint on)
every change to a view's scroll position, which makes debugging scroll
behaviour much easier.
This means those call sites now also get the setters' `< 0` clamps, but
that is behaviour-preserving in every case: each assigned value is
already >= 0. calculateNewOrigin never returns a negative number;
CopyContent copies origins that are themselves always >= 0; and the draw
and scroll writes are all guarded (or fed only non-negative amounts) so
the result can't go below zero.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
refreshMainViews reset the scroll position of every other main view at the very top, before moveMainContextPairToTop runs its CopyContent. CopyContent copies the previously-shown view's content into the now-visible one to avoid a blank frame during the async re-render — but because the reset ran first, it had already zeroed the origin of that soon-to-be-copied source view. The placeholder therefore always appeared scrolled to the top, jumping away from wherever the screen actually was, on every cross-pair transition. Move the reset to after the copy. The end state is unchanged (each other main view still ends at origin 0, and the destination always re-renders), but the brief placeholder now stays at the source view's real scroll position until the real content paints. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When re-rendering content the user was already scrolled into, we want the saved scroll position applied exactly when the real content first paints — not before. Setting the origin up front instead paints it onto whatever placeholder is currently in the view (e.g. the shorter buffer CopyContent left there), which flickers: either a blank frame past the placeholder's end, or a jump to the top when the task resets the origin at startup. Add ViewBufferManager.ScrollToOriginYForNextTask: the next cmd/pty task then (a) does not reset the view to the top at startup even though the command key changed, so the placeholder stays put, (b) sizes its initial read to the saved position so enough content is loaded to fill the view there, and (c) scrolls to it as part of the first refresh, in the same paint that shows the real content. This is the cmd/pty analogue of RenderStringWithScrollTask. No caller sets it yet, so this is behaviour-preserving on its own. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…in view EscapeFromPatchExplorer re-renders the side panel's content back into the main view and wants to land at the scroll position and selection the user had before diving into staging/patch building. The previous version set the origin on the next UI tick, after the placeholder had already been painted at the wrong position, so the restore was visible as a jump. Instead, ask the re-render itself to restore the scroll (via ScrollToOriginYForNextTask), so the saved position is applied in the first paint that shows the real content. The selection still needs the diff loaded down to the selected line, so restore it via ReadToEnd once the content is fully read; the scroll is no longer touched there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Records session 3: the cmd/pty scroll-restore mechanism, the refreshMainViews reset reorder, the corrected onNewKey understanding, and the remaining timing-race investigation to do before productionizing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
30e625a to
f26a998
Compare
…focused main view Diving into staging or patch building from a focused main view (by double-clicking a line, or pressing enter on the selected line) always landed on a single-line selection. Entering the same views through the side panel honours the UseHunkModeInStagingView config and selects the whole hunk by default. The two ways in should agree, so that diving in from the main view feels like the established flow. A non-negative line index in NewState was overloaded for two intents: clicking directly on the patch explorer view (where a single-line range is the start of a drag) and diving in from the main view (where we want the default select mode). Distinguish them with SelectLineInDefaultMode on OnFocusOpts: the main-view entry points set it; the click-to-drag path does not. In hunk mode the selection covers the block of changes around the clicked line. A context line has no surrounding changes, so we snap to the next change line (as toggling hunk mode does); the clicked context line itself is then not part of the selection. This is a separate commit only because the branch is a throwaway prototype; in a real history it would be folded into the commit that introduces the focused-main-view enter behavior rather than landing on top of it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Change the focused main view so that it can have a selection on demand. When clicking in the main view, the clicked line is selected straight away; when pressing
0to focus, no selection is shown initially, as before, but you can enable the selection by pressing<space>; this will select the line in the middle of the view.<esc>dismisses the selection.From there, you can operate on the selected line in various ways:
eto edit it<enter>(or double click) to enter staging/patch buildingGto open the browser with the selected line highlighted in the PR, so you can comment on it.This is only a proof of concept; the big limitation is that the above operations on the selected line
--line-numbers --hyperlinks --hyperlinks-file-link-format="lazygit-edit://{path}:{line}"option is usedI have several ideas how to make this work more generally; some of these require collaboration with the delta developer, and/or support by other pagers.
See #3986 for more information.